Scheduler — Add hiddenDays option to hide arbitrary days of the week#33188
Scheduler — Add hiddenDays option to hide arbitrary days of the week#33188aleksei-semikozov wants to merge 57 commits intoDevExpress:26_1from
Conversation
| const normalizeView = (view: RawViewType): NormalizedView | undefined => (isObject(view) | ||
| ? extend({}, DEFAULT_VIEW_OPTIONS[view.type as string], view) as NormalizedView | ||
| : DEFAULT_VIEW_OPTIONS[view]); | ||
| const normalizeView = ( |
There was a problem hiding this comment.
I think current logic to get normalized hiddenDays value is really over complicated.
I understand that existing mechanism (getViewOption) to get view option in scheduler isn't suitable, because if a view has a default value, then global value wouldn't apply. I think existing mechanism is very awkward and should be refactored.
But in the meantime, I think it is possible to greatly simplify the logic you've added. Here's how:
- add this code to scheduler_options_base_widget.ts:
getViewOption(optionName: string)
if (optionName === 'hiddenDays') {
return normalizeHiddenDaysViewOption(
this.currentView?.type,
this.currentView?.[optionName] ?? this.option(optionName)
);
}
// ...
}-
Remove skipped days default values from DEFAULT_VIEW_OPTIONS
-
Implement normalizeHiddenDaysViewOption:
const normalizeHiddenDaysViewOption = (view: ViewType, hiddenDays?: HiddenDays) => {
const value = hiddenDays ? hiddenDays : defaultValueByView[view];
// other checks and error logging
return value;
}There was a problem hiding this comment.
if need to pass hiddenDays to somewhere, e.g. Header, then just pass it in the config, like 'currentDate' is passed:
private headerConfig(): {
return {
hiddenDays: this.getViewOption('hiddenDays')
// ...
}
}There was a problem hiding this comment.
I am not sure this is a good idea.
It moves hiddenWeekDays normalization to getViewOption(), but that method works only for the current view. Because of that, NormalizedView would no longer be fully normalized, and DEFAULT_VIEW_OPTIONS would no longer be true internal defaults.
Also, the hidden days logic would be spread across different places instead of being resolved once in normalizeView(). So even if the current code is not perfect, this change would make the design less clear, not simpler.
currentDate is just one simple value, so it is easy to normalize in getViewOption().
hiddenWeekDays is more complex. Its final value depends on the view type, per-view value, global value, and default view settings. So it is better to resolve it in normalizeView(), where the view config is built.
| @@ -0,0 +1,64 @@ | |||
| export const isValidWeekday = (value: unknown): value is number => ( | |||
There was a problem hiding this comment.
minor: now we have two names for the same entity: skippedDays and hiddenDays. I would suggest to only use one name in the codebase
There was a problem hiding this comment.
Using skippedDays internally makes sense because skippedDays is a normalized value, while hiddenWeekDays is a public API option and may contain any user-provided input.
| get daysInInterval(): number { | ||
| if (this.skippedDays.length === 0) { | ||
| return this.baseDaysInInterval; | ||
| } | ||
| const visibleDayCount = 7 - this.skippedDays.length; | ||
| if (this.baseDaysInInterval >= 7) { | ||
| return visibleDayCount; | ||
| } | ||
| return this.baseDaysInInterval; | ||
| } |
There was a problem hiding this comment.
Basically what this getter does is:
if currentView is week or workWeek, then return 7 - skippedDays.length
else return 1
Can you simplify this getter to make it more clear?
There was a problem hiding this comment.
Is there a way to check if the currentView is week or workWeek without overriding baseDaysInInterval property in m_view_data_generator_week or m_view_data_generator_work_week?
I mean, something like:
const isWeekLikeView = this.viewType === 'week' || this.viewType === 'workWeek';
return isWeekLikeView
? 7 - this.skippedDays.length
: 1;Thus we wouldn't need baseDaysInInterval prop at all, because in all other viewDataGenerators apart from week don't override the value of baseDaysInInterval
| return this.getFirstDayOfWeek(firstDayOfWeekOption) ?? 0; | ||
| } | ||
|
|
||
| protected getVisibleDayOffset( |
|
|
||
| constructor(public readonly viewType: ViewType) {} | ||
|
|
||
| get daysInInterval(): number { |
There was a problem hiding this comment.
The function is being used here /DevExtreme/packages/devextreme/js/__internal/scheduler/workspaces/m_timeline.ts (421,68):
Property 'daysInInterval' is protected and only accessible within class 'ViewDataGenerator' and its subclasses.
and in tests.
Cant make it protected or private
| public isSkippedDate(date: Date): boolean { | ||
| return isDateSkipped(date, this.skippedDays); | ||
| } |
There was a problem hiding this comment.
Use the same name for the method:
public isDateSkipped(date: Date): boolean {
return isDateSkipped(date, this.skippedDays);
}| const resolveSkippedDays = ( | ||
| viewType: ViewType, | ||
| perViewHiddenWeekDays: unknown, | ||
| globalHiddenWeekDays: number[] | undefined, | ||
| viewDefault: number[], | ||
| ): number[] => { | ||
| const perView = normalizeHiddenWeekDays(perViewHiddenWeekDays); | ||
| if (perView !== undefined) { | ||
| return perView; | ||
| } | ||
| if (globalHiddenWeekDays !== undefined && VIEWS_SUPPORTING_HIDDEN_DAYS.has(viewType)) { | ||
| return normalizeHiddenWeekDays(globalHiddenWeekDays) ?? []; | ||
| } |
There was a problem hiding this comment.
resolveSkippedDays calls normalizeHiddenWeekDays(globalHiddenWeekDays) for every view. When hiddenWeekDays contains all 7 days, this will log W1029 once per view, potentially spamming logs. Consider normalizing the global value once (and logging once) before mapping views, then reusing the normalized result for each view.
Day view Bug $('#scheduler').dxScheduler({
timeZone: 'America/Los_Angeles',
dataSource: data,
views: ['day', 'week', 'workWeek', 'month', {
type: 'day',
name: 'myday',
intervalCount: 7,
}],
hiddenWeekDays: [0, 1, 2, 3, 4, 5],
currentView: 'myday',
currentDate: new Date(2021, 3, 29),
startDayHour: 9,
height: 730,
});Actual result: all week days are visible Expected result: hidden week days shouldn't be visible Looks like we forgot about this case during the spec writing 😔 |
…across scheduler components
I think during chat with the PM the feature support for day view was more like 'nice to have' rather than 'must have'. But I looked at the code and it was rather easy to implement it. I tested, works fine. Added necessary tests. |
No description provided.